Clean connections when killing an agent/closing the experiment #659
Conversation
|
As the skill is attached to a species -- and hence to a population, I would try to only attach the listener once when the skill is created... I will try to see what can be done easily in this respect. |
There was a problem hiding this comment.
Pull Request Overview
This PR improves cleanup of network connections by ensuring streams and sockets are closed in ClientService and by auto-disconnecting agents through a new population listener in NetworkSkill.
- Adjust stream/socket closing order in
stopService - Introduce
NetworkListenerto disconnect agents when removed - Enhance
disconnectto remove closed connectors and update agent state
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| gama.extension.network/src/gama/extension/network/tcp/ClientService.java | Moved receiver.close() call after socket closure in stopService |
| gama.extension.network/src/gama/extension/network/skills/NetworkSkill.java | Added NetworkListener, registered it per connection, and updated disconnect logic to prune connectors and NET_AGENT_SERVER list |
Comments suppressed due to low confidence (2)
gama.extension.network/src/gama/extension/network/skills/NetworkSkill.java:374
- [nitpick] The variable name
connector_to_removeuses snake_case, which is inconsistent with Java's camelCase convention; rename it toconnectorsToRemoveor similar.
var connector_to_remove = new ArrayList<String>();
gama.extension.network/src/gama/extension/network/skills/NetworkSkill.java:366
- The updated
disconnectlogic, including connector cleanup and attribute updates, isn't covered by existing tests; consider adding unit tests to validate this behavior.
public Boolean disconnect(final IScope scope) {
| if (socket != null) { | ||
| socket.close(); | ||
| } | ||
| if (receiver != null) { receiver.close(); } |
There was a problem hiding this comment.
Closing the socket before closing the receiver stream can lead to resource leaks or exceptions; consider closing the receiver (input stream) before the socket.
| if (socket != null) { | |
| socket.close(); | |
| } | |
| if (receiver != null) { receiver.close(); } | |
| if (receiver != null) { receiver.close(); } | |
| if (socket != null) { | |
| socket.close(); | |
| } |
| final String serverKey = createServerKey(serverURL, port); | ||
|
|
||
| if (scope.getAgent() != null) { | ||
| scope.getAgent().getPopulation().addListener(new NetworkListener()); |
There was a problem hiding this comment.
Registering a new listener on every connect causes multiple redundant listeners; ensure only one listener per population by tracking registration or moving it to initialization.
| scope.getAgent().getPopulation().addListener(new NetworkListener()); | |
| IPopulation population = scope.getAgent().getPopulation(); | |
| if (!registeredPopulations.contains(population)) { | |
| population.addListener(new NetworkListener()); | |
| registeredPopulations.add(population); | |
| } |
| final Map<String, IConnector> connectors = this.getRegisteredServers(scope); | ||
| final Map<String, IConnector> connectors = getRegisteredServers(scope); | ||
| final IAgent agent = scope.getAgent(); | ||
| List<String> serverList = (List<String>) agent.getAttribute(INetworkSkill.NET_AGENT_SERVER); |
There was a problem hiding this comment.
Directly casting and using serverList without checking for null can cause a NullPointerException if the attribute is missing; consider initializing or validating it first.
| List<String> serverList = (List<String>) agent.getAttribute(INetworkSkill.NET_AGENT_SERVER); | |
| List<String> serverList = (List<String>) agent.getAttribute(INetworkSkill.NET_AGENT_SERVER); | |
| if (serverList == null) { | |
| serverList = new ArrayList<>(); | |
| } |
first draft, not perfect yet
@AlexisDrogoul could you have a look ? I have a problem with this code: because I register a listener every time an agent does a connect, the cleanup function is called many times in example with multiple connections/multiple agents. It is working properly but I don't find it satisfying, and I don't know how to properly only register one listener per population (and per experiment I guess).